Skip to content

fix(effects): paginate FHIR and REST read-backs and verify total count (#1476) - #1511

Closed
yunusarda-sudo wants to merge 2 commits into
OpenAdaptAI:mainfrom
yunusarda-sudo:fix/fhir-rest-readback-paging
Closed

yunusarda-sudo wants to merge 2 commits into
OpenAdaptAI:mainfrom
yunusarda-sudo:fix/fhir-rest-readback-paging

Conversation

@yunusarda-sudo

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix

Summary

Fixes #1476 (docs/audits/REALISM_AUDIT.md Rank 11).

In openadapt_flow/runtime/effects/fhir.py and rest.py, effect verifiers previously inspected only the initial page returned by the system-of-record search query.

When a FHIR server or REST endpoint paginates search results (e.g. at 20–50 items), records situated on page 2 and beyond were completely invisible to the read-back. This led to safety flaws:

  1. False Confirmation of Uniqueness / Count: A duplicate record pushed to page 2 evaded detection, incorrectly confirming expected_count=1.
  2. False Absence Confirmation: An absence check would confirm when a record was actually present on page 2.
  3. Ignored Total Discrepancies: When servers provide Bundle.total or JSON total / total_count, discrepancies between read records and reported totals were ignored.

Changes

  1. FHIR Pagination (openadapt_flow/runtime/effects/fhir.py):

    • In FhirEffectVerifier._search(), parse Bundle.link for relation "next".
    • Iteratively follow next URLs up to a configurable safety limit (max_pages=50, default).
    • Verify expected_total (from Bundle.total) against the count of flattened resources read.
    • If total differs from len(out) or if pagination stops prematurely, return None so judge_records returns INDETERMINATE.
  2. REST Pagination (openadapt_flow/runtime/effects/rest.py):

    • In RestRecordVerifier._fetch_records(), inspect RFC 5988 / 8288 standard Link: <url>; rel="next" header (via resp.links["next"]["url"]) and JSON body attributes (next, next_url, links.next).
    • Iteratively follow subsequent pages up to max_pages=50.
    • Verify expected_total (body["total"]\ or body["total_count"]) against accumulated records, returning None (INDETERMINATE) on mismatch.
  3. Test Coverage:

    • Extended tests/_fhir_fake.py with _count, _offset, and _bad_total query parameters.
    • Added regression test suite in tests/test_effect_fhir.py verifying multi-page accumulation, absence detection past page 1, and total mismatch rejection.
    • Added regression test suite in tests/test_rest_verifier_transport.py covering header Link pagination, body JSON link pagination, and total mismatch handling.

Checklist

  • I have tested this change locally (pytest packages/flow/tests/test_effect_fhir.py packages/flow/tests/test_rest_verifier_transport.py -> 17 passed, 1 skipped)
  • Documentation is updated if needed
  • CI passes (ruff check clean)

Notes

  • max_pages defaults to 50 as a protective bounded loop against infinite server cycles.
  • When total does not match the retrieved count, read-back treats the evidence as indeterminate rather than making false claims.

- Follow FHIR Bundle.link[rel=next] across pages up to max_pages cap
- Support REST pagination via RFC 5988 Link header and body next fields
- Verify Bundle.total and REST total/total_count against records read, returning INDETERMINATE on mismatch
- Add pagination and total validation test coverage for FHIR and REST verifiers
- Resolves OpenAdaptAI#1476 (realism audit rank 11)
Signed-off-by: Yunus Ardaguney <yunusarda-sudo@users.noreply.github.com>
@abrichr

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member

Thank you for this, it's a real gap from our audit (#1476), and your fail-closed handling of failed pages, the page cap and mismatched totals is right.

We're closing it as superseded by #1520, which rewrites the same two functions (FhirEffectVerifier._search and RestRecordVerifier._fetch_records), so the two would conflict. #1520 also covers two issues we'd otherwise have asked you to fix here:

  • next links are followed only on the same origin, so the Authorization header is never sent to another host;
  • REST paging uses an explicit records_next_path setting instead of inferring next/total from field names, which could misread unrelated fields such as a total amount in a claims payload.

We'll credit you in #1520 and the changelog. Your #1515 is approved and queued. Thanks again, and contributions like these are very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FHIR and REST read-backs read only the first page

2 participants